-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review all code up to this point #4
base: main
Are you sure you want to change the base?
Conversation
generate embedding torch.tensor numpy warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot to cover in this PR. I've managed to only get through a few files. I'll go through the rest tomorrow and provide more feedback.
""" | ||
if name == "gaussian": | ||
|
||
def gaussian(x, mean, sigma): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend refactoring this into a separate class, perhaps called GaussianStrategy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly with other strategies.
def gaussian(x, mean, sigma): | ||
a = 1 / (sigma * np.sqrt(2 * np.pi)) | ||
exp_val = -(1 / 2) * (((x - mean) ** 2) / (sigma ** 2)) | ||
tmp = list(a * np.exp(exp_val))[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using result
rather than tmp
.
sigma = 0.2 | ||
return [gaussian(conf_i, mean, sigma) for conf_i in confidences] | ||
|
||
def get_samples(image_list, confidences, number_of_samples): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: number_of_samples
could be renamed as sample_count
to avoid the need for a preposition.
) | ||
return picked_samples | ||
|
||
def get_index(samples, dataset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is called get_index
, but the result appears to be an element in dataset
.
|
||
def get_index(samples, dataset): | ||
data_index = [] | ||
for i in samples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the map
function in place of this for loop as follows:
data_index = map(lambda x: dataset.index(x), samples)
super().__init__() | ||
""" | ||
Keyword arguments | ||
n_input -- Size of the input embeddings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hungarian notation is typically discouraged for variable naming. Consider changing these to input_size
, num_classes
or class_count
, etc.
class SSLEvaluator(nn.Module): | ||
"""The classification model which is used with the SSL encoder""" | ||
|
||
def __init__(self, n_input, n_classes, n_hidden=512, p=0.1, c_type="Sigmoid"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worthwhile making classification type into an enum.
@@ -0,0 +1,46 @@ | |||
import torch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider maintaining a consistent folder and file naming structure. Typically in Python, file and folder names are all lowercase and underscores are used to separate words.
class SimCLRTransform(Pipeline): | ||
def __init__( | ||
self, | ||
DATA_PATH, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All caps should ideally only be used for constants.
References:
PEP8
Python naming convention
image = self.swapaxes(image) | ||
return image | ||
|
||
def define_graph(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function returns a batch. Would be helpful to have the name of the function be more descriptive of what it returns.
No description provided.